Conversation
| await idempotence_helper.idempotent_update(conn, clear_session) | ||
|
|
||
|
|
||
| class PostgresSession(SessionABC): |
There was a problem hiding this comment.
I think I saw you aren't supposed to use SessionABC, but the underlying protocol
There was a problem hiding this comment.
That's probably better.
There was a problem hiding this comment.
Does OpenAI provide any session implementations themselves here? Can we just write a DurableSession that wraps those instead of having to do all of the DB stuff ourselves? A DurableSession that accepts a user-provided session means they won't be limited to just specific implementations we implement.
There was a problem hiding this comment.
I like this suggestion. We might be able to use SQLAlchemySession
There was a problem hiding this comment.
Or SQLiteSession since this is just a sample. I now think we should offer ActivityBasedSession that is a wrapper around third-party sessions and InWorkflowSession that is just a list implementation of session. Granted I know this is just a sample but it can save you from having a bunch of DB details in the PR while also making it clear the two approaches.
Btw, it seems the SQLiteSession is in memory by default so it would work in workflows given some sandbox avoidance work, but I think a more naive in-memory implementation makes sense over taking a SQLite dependency for what amounts to a very simple list interface.
| self.session_id = session_id | ||
| self.config = config | ||
|
|
||
| async def get_items(self, limit: int | None = None) -> list[TResponseInputItem]: |
There was a problem hiding this comment.
If all of these items will end up in history anyways as a result of this call, is there any value in using a separate database instead of in-memory? Arguably it's actually worse to put the result of every call to this method in history because it may be called multiple times I assume.
There was a problem hiding this comment.
Storing the conversation in both the workflow's event history and the database is somewhat redundant.
Some users want to retain a record of the conversation in a database, and this implementation meets that need. We might be able to optimize performance by deferring the database writes to when the workflow ends. However that means it isn't immediately available and will be missing if the workflow doesn't finish for some reason. The current approach defers such optimizations.
We have also explored getting the session chat history out of the workflow's event history and instead keeping references to the session stored in Postgres. However, making that work requires more changes to OpenAI Agents SDK.
There was a problem hiding this comment.
Storing the conversation in both the workflow's event history and the database is somewhat redundant. [...] Some users want to retain a record of the conversation in a database, and this implementation meets that need
Makes sense, mentioned in other comment, but I think we should (also?) provide a literal InMemorySession/InWorkflowSession that is just in-memory list by default since we know that memory is durable in Temporal. I wish OpenAI would provide this in-memory session implementation, but it makes sense they don't because their memory is not durable and sessions is a naive attempt at some durability I assume.
3 participants
What was changed
Why?
Checklist
Closes
How was this tested: